Composite multiple layers in Windows software rendering #51759

yaakovschectman · 2024-03-28T19:30:52Z

Blend pixels per-alpha when presenting multiple layers from the software compositor.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

loic-sharma · 2024-03-29T22:07:04Z

shell/platform/windows/compositor_software.cc

+
+  int width = x_max - x_min;
+  int height = y_max - y_min;
+  std::vector<uint32_t> allocation(width * height);


Can we avoid this allocation if there's only 1 layer?

loic-sharma · 2024-03-29T22:08:42Z

shell/platform/windows/compositor_software.cc

-
-  return view->PresentSoftwareBitmap(
-      backing_store.allocation, backing_store.row_bytes, backing_store.height);
+  int x_min = INT_MAX;


Could we split this up and have a helper for the compositing step?

loic-sharma · 2024-03-29T22:10:22Z

shell/platform/windows/compositor_software_unittests.cc

+// Overlay layer has pixel values:
+// RED: 127   WHITE: 0
+// BLUE: 127  BLACK: 255
+TEST_F(CompositorSoftwareTest, PresentMultiLayers) {


Could we also have a test for layers that don't overlap fully?

yaakovschectman · 2024-04-01T16:56:31Z

Good idea. I'll get on that, thanks

…

On Fri, Mar 29, 2024, 6:10 PM Loïc Sharma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In shell/platform/windows/compositor_software_unittests.cc <#51759 (comment)>: > @@ -162,5 +162,57 @@ TEST_F(CompositorSoftwareTest, HeadlessPresentIgnored) { ASSERT_TRUE(compositor.CollectBackingStore(&backing_store)); } +// Test compositing an upper layer on a base layer, each 2x2 pixels. +// Base layer has opaque pixel values: +// BLACK RED +// GREEN WHITE +// Overlay layer has pixel values: +// RED: 127 WHITE: 0 +// BLUE: 127 BLACK: 255 +TEST_F(CompositorSoftwareTest, PresentMultiLayers) { Could we also have a test for layers that don't overlap fully? — Reply to this email directly, view it on GitHub <#51759 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2AOOLGAG435QHVIPVUGYB3Y2XRGHAVCNFSM6AAAAABFNLUONOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRZGY2DQNJXGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

yaakovschectman · 2024-04-01T16:56:51Z

I think so. I'll break it up a bit

…

On Fri, Mar 29, 2024, 6:09 PM Loïc Sharma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In shell/platform/windows/compositor_software.cc <#51759 (comment)>: > @@ -51,18 +51,77 @@ bool CompositorSoftware::Present(FlutterViewId view_id, return view->ClearSoftwareBitmap(); } - // TODO: Support compositing layers and platform views. - // See: flutter/flutter#31713 - FML_DCHECK(layers_count == 1); - FML_DCHECK(layers[0]->offset.x == 0 && layers[0]->offset.y == 0); - FML_DCHECK(layers[0]->type == kFlutterLayerContentTypeBackingStore); - FML_DCHECK(layers[0]->backing_store->type == - kFlutterBackingStoreTypeSoftware); - - const auto& backing_store = layers[0]->backing_store->software; - - return view->PresentSoftwareBitmap( - backing_store.allocation, backing_store.row_bytes, backing_store.height); + int x_min = INT_MAX; Could we split this up and have a helper for the compositing step? — Reply to this email directly, view it on GitHub <#51759 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2AOOLAG3AJ6BSSHTALG2WDY2XRABAVCNFSM6AAAAABFNLUONOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRZGY2DOOBQG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

yaakovschectman · 2024-04-01T16:57:39Z

May need some extra logic once we implement platform views but yeah I think it's doable like that

…

On Fri, Mar 29, 2024, 6:07 PM Loïc Sharma ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In shell/platform/windows/compositor_software.cc <#51759 (comment)>: > + int x_max = INT_MIN; + int y_min = INT_MAX; + int y_max = INT_MIN; + for (const FlutterLayer** layer = layers; layer < layers + layers_count; + layer++) { + auto& offset = (*layer)->offset; + auto& size = (*layer)->size; + x_min = std::min(x_min, static_cast<int>(offset.x)); + y_min = std::min(y_min, static_cast<int>(offset.y)); + x_max = std::max(x_max, static_cast<int>(offset.x + size.width)); + y_max = std::max(y_max, static_cast<int>(offset.y + size.height)); + } + + int width = x_max - x_min; + int height = y_max - y_min; + std::vector<uint32_t> allocation(width * height); Can we avoid this allocation if there's just a single layer? — Reply to this email directly, view it on GitHub <#51759 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A2AOOLC3IPZGCTJ6OB6PBYTY2XQZ5AVCNFSM6AAAAABFNLUONOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRZGY2DKNJYGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

cbracken

Overall looks good - just a few comments!

shell/platform/windows/compositor_software.cc

cbracken · 2024-04-04T21:02:33Z

shell/platform/windows/compositor_software.cc

+  auto& offset = layer.offset;
+  auto& size = layer.size;
+
+  for (int y_src = 0; y_src < size.height; y_src++) {


Will take a closer look, but first thoughts:

but it feels like it should be possible to update the loop conditions to avoid the two < 0, >= height checks below.

this feels like there should be a way to parallelise this a bit

I suppose the obvious way to parallelize this would be to use the GPU, but this is the software compositor, so that seems self defeating.
Also, I will test adjusting the loop statements once it rebuilds.

Feel free to check now.

I suspect the right way to parallelize would be to vectorize this code if possible... I wonder if Windows has any helpers that can do efficient bitmap blending? If not, that seems like too much work for an MVP.

The only ones I'm familiar with work with HDCs instead of pixel data in memory, and so we would need to create those new DCs per layer to apply them as far as I understand

shell/platform/windows/compositor_software.cc

Co-authored-by: Loïc Sharma <[email protected]>

loic-sharma · 2024-04-10T20:13:38Z

shell/platform/windows/compositor_software.cc

+  int x_min = INT_MAX;
+  int x_max = INT_MIN;
+  int y_min = INT_MAX;
+  int y_max = INT_MIN;
+  for (const FlutterLayer** layer = layers; layer < layers + layers_count;
+       layer++) {
+    const FlutterPoint& offset = (*layer)->offset;
+    const FlutterSize& size = (*layer)->size;
+    // FlutterPoint and FlutterSize store coordinates as doubles.
+    // Coordinates must be truncated to integers to represent whole pixels.
+    x_min = std::min(x_min, static_cast<int>(offset.x));
+    y_min = std::min(y_min, static_cast<int>(offset.y));
+    x_max = std::max(x_max, static_cast<int>(offset.x + size.width));
+    y_max = std::max(y_max, static_cast<int>(offset.y + size.height));
+  }
+
+  int width = x_max - x_min;
+  int height = y_max - y_min;


Could we add a helper for the logic to calculate the position/size of the present?

loic-sharma · 2024-04-10T20:15:27Z

shell/platform/windows/compositor_software.cc

+                                     width * 4, height);
+}
+
+void CompositorSoftware::BlendLayer(std::vector<uint32_t>& allocation,


Instead of an instance method that's declared in the header, should we make this be a top-level function in an anonymous namespace in this file?

In my mind the header file is best for public APIs or methods that need to interact with instance fields. However, this helper is self-contained logic that doesn't require any instance members.

loic-sharma · 2024-04-10T21:43:10Z

shell/platform/windows/compositor_software.cc

-  return view->PresentSoftwareBitmap(
-      backing_store.allocation, backing_store.row_bytes, backing_store.height);
+  return view->PresentSoftwareBitmap(static_cast<void*>(allocation.data()),
+                                     width * 4, height);


For someone not familiar with the bitmap allocation, this 4 might look like a magic value. Could we add a constant for the 4 with a quick comment for clarity?

loic-sharma

LGTM, excellent work!

cbracken

Pass two: lgtm! Awesome stuff!

…utter/engine#51759)

…146645) flutter/engine@d8560d4...98a8ad1 2024-04-11 [email protected] [et] Correctly plumb usage line length limit (flutter/engine#52039) 2024-04-11 [email protected] Revert "Roll Dart SDK from 3d13dbfb3284 to 764bdb7d0344 (1 revision) (#52051)" (flutter/engine#52060) 2024-04-11 [email protected] Composite multiple layers in Windows software rendering (flutter/engine#51759) 2024-04-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 764bdb7d0344 to 0219e897c6ac (1 revision) (#52053)" (flutter/engine#52058) 2024-04-11 [email protected] Roll Skia from 2a2fe4303507 to 1dc3c2c1b550 (1 revision) (flutter/engine#52055) 2024-04-11 [email protected] Roll Skia from aa30d76a345f to 2a2fe4303507 (1 revision) (flutter/engine#52054) 2024-04-11 [email protected] Roll Dart SDK from 764bdb7d0344 to 0219e897c6ac (1 revision) (flutter/engine#52053) 2024-04-11 [email protected] Roll Skia from 29f0c9d84e70 to aa30d76a345f (1 revision) (flutter/engine#52052) 2024-04-11 [email protected] Roll Dart SDK from 3d13dbfb3284 to 764bdb7d0344 (1 revision) (flutter/engine#52051) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#146645) flutter/engine@d8560d4...98a8ad1 2024-04-11 [email protected] [et] Correctly plumb usage line length limit (flutter/engine#52039) 2024-04-11 [email protected] Revert "Roll Dart SDK from 3d13dbfb3284 to 764bdb7d0344 (1 revision) (flutter#52051)" (flutter/engine#52060) 2024-04-11 [email protected] Composite multiple layers in Windows software rendering (flutter/engine#51759) 2024-04-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Dart SDK from 764bdb7d0344 to 0219e897c6ac (1 revision) (flutter#52053)" (flutter/engine#52058) 2024-04-11 [email protected] Roll Skia from 2a2fe4303507 to 1dc3c2c1b550 (1 revision) (flutter/engine#52055) 2024-04-11 [email protected] Roll Skia from aa30d76a345f to 2a2fe4303507 (1 revision) (flutter/engine#52054) 2024-04-11 [email protected] Roll Dart SDK from 764bdb7d0344 to 0219e897c6ac (1 revision) (flutter/engine#52053) 2024-04-11 [email protected] Roll Skia from 29f0c9d84e70 to aa30d76a345f (1 revision) (flutter/engine#52052) 2024-04-11 [email protected] Roll Dart SDK from 3d13dbfb3284 to 764bdb7d0344 (1 revision) (flutter/engine#52051) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

yaakovschectman added 4 commits March 26, 2024 15:27

Layers

8672c29

Unit test

c50fde4

Check for sizes or offsets

06b27c8

Formatting

ce31f49

yaakovschectman added the affects: desktop label Mar 28, 2024

yaakovschectman requested review from cbracken, loic-sharma, dkwingsmt and gspencergoog March 28, 2024 19:30

yaakovschectman self-assigned this Mar 28, 2024

github-actions bot added the platform-windows label Mar 28, 2024

Merge branch 'main' into software_layers

4836c57

yaakovschectman marked this pull request as ready for review March 29, 2024 18:31

loic-sharma reviewed Mar 29, 2024

View reviewed changes

yaakovschectman and others added 6 commits April 2, 2024 09:36

Merge branch 'main' into software_layers

09a7265

Handle single layer

0c3640f

Refactor blending

f35f8dd

Unit test

b7d0d19

Formatting

0237513

Merge branch 'main' into software_layers

9385e81

yaakovschectman requested a review from loic-sharma April 3, 2024 18:10

cbracken reviewed Apr 4, 2024

View reviewed changes

Merge branch 'main' into software_layers

bfb94f0

cbracken reviewed Apr 4, 2024

View reviewed changes

PR Feedback

e012e1e

Adjust loop bounds

1d11b21

loic-sharma reviewed Apr 5, 2024

View reviewed changes

shell/platform/windows/compositor_software.cc Outdated Show resolved Hide resolved

yaakovschectman and others added 4 commits April 9, 2024 16:45

Merge branch 'main' into software_layers

4361321

Update shell/platform/windows/compositor_software.cc

b0835af

Co-authored-by: Loïc Sharma <[email protected]>

Build

0dba35b

Merge branch 'main' into software_layers

b64637d

yaakovschectman requested review from cbracken and loic-sharma April 10, 2024 20:00

loic-sharma reviewed Apr 10, 2024

View reviewed changes

PR feedback

27d3405

loic-sharma reviewed Apr 10, 2024

View reviewed changes

loic-sharma approved these changes Apr 10, 2024

View reviewed changes

Use sizeof

e2d7bbb

cbracken approved these changes Apr 10, 2024

View reviewed changes

yaakovschectman merged commit fb2722c into flutter:main Apr 11, 2024
26 checks passed

yaakovschectman deleted the software_layers branch April 11, 2024 14:25

engine-flutter-autoroll mentioned this pull request Apr 11, 2024

Roll Flutter Engine from d8560d495d9f to fb2722c57527 (7 revisions) flutter/flutter#146641

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2024

fb2722c57 Composite multiple layers in Windows software rendering (fl…

8830e15

…utter/engine#51759)

engine-flutter-autoroll mentioned this pull request Apr 11, 2024

Roll Flutter Engine from d8560d495d9f to ffb344d17901 (8 revisions) flutter/flutter#146643

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2024

fb2722c57 Composite multiple layers in Windows software rendering (fl…

d236194

…utter/engine#51759)

engine-flutter-autoroll mentioned this pull request Apr 11, 2024

Roll Flutter Engine from d8560d495d9f to 98a8ad144597 (9 revisions) flutter/flutter#146645

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 11, 2024

fb2722c57 Composite multiple layers in Windows software rendering (fl…

6ac7446

…utter/engine#51759)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composite multiple layers in Windows software rendering #51759

Composite multiple layers in Windows software rendering #51759

yaakovschectman commented Mar 28, 2024

loic-sharma Mar 29, 2024 •

edited

Loading

loic-sharma Mar 29, 2024

loic-sharma Mar 29, 2024

yaakovschectman commented Apr 1, 2024 via email

yaakovschectman commented Apr 1, 2024 via email

yaakovschectman commented Apr 1, 2024 via email

cbracken left a comment

cbracken Apr 4, 2024

yaakovschectman Apr 4, 2024 •

edited

Loading

yaakovschectman Apr 5, 2024

loic-sharma Apr 5, 2024 •

edited

Loading

yaakovschectman Apr 10, 2024

loic-sharma Apr 10, 2024

loic-sharma Apr 10, 2024

loic-sharma Apr 10, 2024

loic-sharma left a comment

cbracken left a comment

Composite multiple layers in Windows software rendering #51759

Composite multiple layers in Windows software rendering #51759

Conversation

yaakovschectman commented Mar 28, 2024

Pre-launch Checklist

loic-sharma Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaakovschectman commented Apr 1, 2024 via email

yaakovschectman commented Apr 1, 2024 via email

yaakovschectman commented Apr 1, 2024 via email

cbracken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaakovschectman Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic-sharma left a comment

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

loic-sharma Mar 29, 2024 •

edited

Loading

yaakovschectman Apr 4, 2024 •

edited

Loading

loic-sharma Apr 5, 2024 •

edited

Loading